Apply rustfmt. #385

marshallpierce · 2016-12-20T17:59:07Z

Various broken cases handled manually.
Also, add a Travis hook and a sample pre-commit hook.
(Addresses #285)

Various broken cases handled manually. Also, add a Travis hook and a sample pre-commit hook.

marshallpierce · 2016-12-20T18:01:22Z

Aesthetically I don't necessarily like some of the edits rustfmt applies, but if rustfmt checks via CI are the goal (and I do like consistent formatting), then I don't see a way around doing what it wants.

I agree to license my contributions to each file under the terms given at the top of each file I changed.

marshallpierce · 2016-12-20T17:59:57Z

examples/checkdigest.rs

@@ -18,16 +18,20 @@ use ring::*;
 use std::error::Error;
 use std::io::{Read, Write};

-
+#[cfg_attr(rustfmt, rustfmt_skip)]


rustfmt handles multi line string literals poorly at the moment

marshallpierce · 2016-12-20T18:04:44Z

src/aead/chacha20_poly1305_openssh.rs

@@ -26,7 +26,8 @@
 //! `padding_length||payload||random padding`.
 //!
 //! [[email protected]]:
-//!    http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL.chacha20poly1305?annotate=HEAD
+//!    http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/\


This url-line-breaking is pretty unfortunate but I didn't see any other way to appease rustfmt.

Let's fix rustfmt instead. I think that keeping URLs intact should be an uncontroversial change to rustfmt, at least as an option.

Already a bug: rust-lang/rustfmt#506

marshallpierce · 2016-12-20T18:06:52Z

src/der.rs

-pub fn positive_integer<'a>(input: &mut untrusted::Reader<'a>)
-                            -> Result<untrusted::Input<'a>, error::Unspecified> {
+pub fn positive_integer<'a>
+    (input: &mut untrusted::Reader<'a>)


This formatting is also very strange

Yes, it seems like maybe a rustfmt bug? When it does this to a where clause it puts the opening brace on a new line. Not sure why it doesn't do that in this case.

marshallpierce · 2016-12-20T18:07:31Z

src/digest/digest.rs

            }
            // We don't increase |self.completed_data_blocks| because the
            // padding isn't data, and so it isn't included in the data length.
            padding_pos = 0;
        }

-        polyfill::slice::fill(
-            &mut self.pending[padding_pos..(self.algorithm.block_len - 8)], 0);
+        let slice = self.pending[padding_pos..(self.algorithm.block_len - 8)];


the inline calculation in the slice confused rustfmt mightily so I pulled out a local

marshallpierce · 2016-12-20T18:10:40Z

src/ec/suite_b/ops/p256.rs

-                                a: *const Limb/*[COMMON_OPS.num_limbs]*/);
-    fn GFp_p256_scalar_sqr_rep_mont(r: *mut Limb/*[COMMON_OPS.num_limbs]*/,
-                                    a: *const Limb/*[COMMON_OPS.num_limbs]*/,
+    fn GFp_nistz256_add(// [COMMON_OPS.num_limbs]


Had to make these // comments so that it wouldn't try to put them on the same line, and then fall afoul of the line length limit

It seems better to just tell rustfmt to ignore this block.

marshallpierce · 2016-12-20T18:12:54Z

src/pbkdf2.rs

@@ -210,6 +211,7 @@ fn derive_block(secret: &hmac::SigningKey, iterations: usize, salt: &[u8],
 ///
 /// `derive` panics if `out.len()` is larger than (2**32 - 1) * the PRF digest
 /// length, per the PBKDF2 specification.
+#[cfg_attr(rustfmt, rustfmt_skip)]


This function also made rustfmt mad.

marshallpierce · 2016-12-20T18:14:40Z

src/poly1305.rs

+    let aligned_buf = slice_as_array_ref_mut!(&mut buf[offset..(offset +
+                                                                OPAQUE_LEN)],
+                                              OPAQUE_LEN)
+        .unwrap();


briansmith

I already tried to do this. I would very much like to do so, and I don't think I have particularly strong preferences for code formatting in general, but IMO rustfmt is doing unreasonable things to the code.

I suggest we just punt on this for now, though if you want to go through and resubmit the no-brainer changes, I would review that.

briansmith · 2016-12-21T04:04:22Z

src/aead/aes_gcm.rs

+                         tag,
+                         nonce,
+                         ad.as_ptr(),
+                         ad.len())


This is most of the reason why we don't use rustfmt already. We need a formatting tool that it can format function calls in the compact format, and that generally optimizes for readability first and minimum number of lines second, like the Google clang-format style.

Alternatively, we might be able to use the default rustfmt style after we've oxidized more of ring so that there aren't so many function calls that take a large number of arguments. But not yet.

briansmith · 2016-12-21T04:05:08Z

src/aead/aes_gcm.rs

    })
 }

 fn aes_gcm_open(ctx: &[u64; aead::KEY_CTX_BUF_ELEMS],
                nonce: &[u8; aead::NONCE_LEN], in_out: &mut [u8],
                in_prefix_len: usize, tag_out: &mut [u8; aead::TAG_LEN],
-                ad: &[u8]) -> Result<(), error::Unspecified> {
+                ad: &[u8])
+                -> Result<(), error::Unspecified> {


This is not such a big deal as the function call formatting, but still it seems unnecessary.

briansmith · 2016-12-21T04:07:42Z

src/aead/chacha20_poly1305_openssh.rs

@@ -26,7 +26,8 @@
 //! `padding_length||payload||random padding`.
 //!
 //! [[email protected]]:
-//!    http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL.chacha20poly1305?annotate=HEAD
+//!    http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/\


Let's fix rustfmt instead. I think that keeping URLs intact should be an uncontroversial change to rustfmt, at least as an option.

briansmith · 2016-12-21T04:12:00Z

src/der.rs

-pub fn positive_integer<'a>(input: &mut untrusted::Reader<'a>)
-                            -> Result<untrusted::Input<'a>, error::Unspecified> {
+pub fn positive_integer<'a>
+    (input: &mut untrusted::Reader<'a>)


Yes, it seems like maybe a rustfmt bug? When it does this to a where clause it puts the opening brace on a new line. Not sure why it doesn't do that in this case.

briansmith · 2016-12-21T04:13:14Z

src/der.rs

@@ -221,7 +223,8 @@ mod tests {
          &[0x02, 0x00, 0x01],
          &[0x02, 0x01],
          &[0x02, 0x01, 0x00, 0x01],
-          &[0x02, 0x01, 0x01, 0x00], // Would be valid if last byte is ignored.
+          /* Would be valid if last byte is ignored. */
+          &[0x02, 0x01, 0x01, 0x00],


Presumably this could be written with a C++ comment in that position instead of a C-style comment?

briansmith · 2016-12-21T04:17:56Z

src/ec/suite_b/ops/p256.rs

-                                a: *const Limb/*[COMMON_OPS.num_limbs]*/);
-    fn GFp_p256_scalar_sqr_rep_mont(r: *mut Limb/*[COMMON_OPS.num_limbs]*/,
-                                    a: *const Limb/*[COMMON_OPS.num_limbs]*/,
+    fn GFp_nistz256_add(// [COMMON_OPS.num_limbs]


It seems better to just tell rustfmt to ignore this block.

marshallpierce · 2016-12-21T14:47:09Z

I'm fine with punting. I'll close this (and submit some rustfmt bugs). I just picked it up because it looked like something I could get done while you looked at my other PR's. ;) It's not like this project had egregiously sloppy code formatting to begin with, so the win isn't very significant when it comes with so many annoying mis-formats.

Apply rustfmt.

91cafac

Various broken cases handled manually. Also, add a Travis hook and a sample pre-commit hook.

marshallpierce added 2 commits December 20, 2016 12:09

Fix quotes on travis invocation of rustfmt

b6adda0

I agree to license my contributions to each file under the terms given at the top of each file I changed.

Reverse comment order to better express original intent.

37ee113

I agree to license my contributions to each file under the terms given at the top of each file I changed.

marshallpierce commented Dec 20, 2016

View reviewed changes

briansmith requested changes Dec 21, 2016

View reviewed changes

marshallpierce closed this Dec 21, 2016

briansmith mentioned this pull request Feb 3, 2017

Integrate rustfmt into the build system and/or Continuous Integration #285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply rustfmt. #385

Apply rustfmt. #385

marshallpierce commented Dec 20, 2016

marshallpierce commented Dec 20, 2016

marshallpierce Dec 20, 2016

marshallpierce Dec 20, 2016

briansmith Dec 21, 2016

marshallpierce Dec 21, 2016

marshallpierce Dec 20, 2016

briansmith Dec 21, 2016

marshallpierce Dec 20, 2016

marshallpierce Dec 20, 2016

briansmith Dec 21, 2016

marshallpierce Dec 20, 2016

marshallpierce Dec 20, 2016

briansmith left a comment

briansmith Dec 21, 2016

briansmith Dec 21, 2016

briansmith Dec 21, 2016

briansmith Dec 21, 2016

briansmith Dec 21, 2016

briansmith Dec 21, 2016

marshallpierce commented Dec 21, 2016

Apply rustfmt. #385

Apply rustfmt. #385

Conversation

marshallpierce commented Dec 20, 2016

marshallpierce commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marshallpierce commented Dec 21, 2016